Introduce `FundingTransactionReadyForSignatures` event #3889

dunxen · 2025-06-24T12:12:50Z

Cherry-picked from #3735 as it is relevant to splicing and will unblock testing after #3736 lands.

The FundingTransactionReadyForSignatures event requests witnesses from the client for their contributed inputs to an interactively constructed transaction.

The client calls ChannelManager::funding_transaction_signed to provide the witnesses to LDK.

ldk-reviews-bot · 2025-06-24T12:12:52Z

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

jkczyz · 2025-06-24T16:31:27Z

lightning/src/ln/channelmanager.rs

+		let witnesses: Vec<_> = transaction
+			.input
+			.into_iter()
+			.filter_map(|input| if input.witness.is_empty() { None } else { Some(input.witness) })
+			.collect();


Don't have a strong opinion here, but seems we can avoid this by passing the Transaction through and only collecting witnesses when we are ready to construct TxSignatures.

Cool, yeah will look at changing this after fixups.

lightning/src/ln/interactivetxs.rs

codecov · 2025-06-25T11:08:53Z

Codecov Report

Attention: Patch coverage is 9.70874% with 93 lines in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (6771d84) to head (ff1489d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	1.85%	52 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs	11.11%	32 Missing ⚠️
lightning/src/ln/interactivetxs.rs	45.45%	6 Missing ⚠️
lightning/src/events/mod.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3889      +/-   ##
==========================================
- Coverage   88.88%   88.82%   -0.07%     
==========================================
  Files         165      165              
  Lines      118886   118971      +85     
  Branches   118886   118971      +85     
==========================================
  Hits       105676   105676              
- Misses      10892    10976      +84     
- Partials     2318     2319       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-reviews-bot · 2025-06-26T12:13:27Z

🔔 1st Reminder

Hey @wpaulino @optout21! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-26T12:13:29Z

🔔 1st Reminder

Hey @wpaulino @optout21! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

optout21

Looks good!

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-06-28T12:14:21Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-30T12:15:10Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz

Not sure if there was a problem rebasing, but some comments that had been marked resolved weren't fixed.

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

lightning/src/events/mod.rs

dunxen · 2025-07-01T08:08:04Z

Not sure if there was a problem rebasing, but some comments that had been marked resolved weren't fixed.

Yeah, they got lost on a rebase and somehow lost the commit. Rebased to get the one CI fix in. Fixing.

ldk-reviews-bot · 2025-07-02T12:15:51Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-07-05T00:01:03Z

🔔 5th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-07-07T00:01:47Z

🔔 6th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/events/mod.rs

wpaulino · 2025-07-07T19:08:26Z

lightning/src/ln/channel.rs

+	fn verify_interactive_tx_signatures(&mut self, _witnesses: &Vec<Witness>) {
+		if let Some(ref mut _signing_session) = self.interactive_tx_signing_session {
+			// Check that sighash_all was used:
+			// TODO(dual_funding): Check sig for sighash


Any reason this isn't done yet?

Oh yeah we're actually unable to do this reliably, especially if inputs were spend P2WSH. The public comments warn the client to ensure it is SIGHASH_ALL (or SIGHASH_DEFAULT/SIGHASH_ALL) if spending a P2TR UTXO.

These TODOs need to be removed.

We should be able to use ScriptBuf::instructions to identify all pushes that match a signature length, parse those that match, and make sure they come with SIGHASH_ALL.

I'm worried we assume something is a sig just based on length when it's not. I mean it's probably a sig, but we'd then actually need to check if there is a corresponding checksig in the script.

If it wasn't a sig then we'd always treat that as invalid if it happens to have the same length and something that looks like a sighash_all byte (or the case no explicit sighash as in p2tr sighash_default).

It's probably unlikely but I haven't considered how "probably".

For ECDSA sigs we can be a bit more sure since they have to parse as DER-encoded. P2WSH and P2TR-script-spend always reveal the witness script so we can certainly check for corresponding OP_CHECKSIGs there. P2WPKH and P2TR-key-spend have a specific index within the witness that corresponds to the signature.

If we don't check this for holder witnesses, then we're susceptible to malleability attacks.

We checked that all prev outputs are "standard" during tx construction, so if a witness is not one of the recognised witnesses shall we just ignore and not error? (considering it'll fail anyway). I'd say we should only care about checking for sighash types for witnesses corresponding to "standard" scriptpubkeys.

We shouldn't ever get to this point if that were the case, we should return an error when the inputs are provided.

The function name should probably include "sighash_type" to make it clear that's all we check.

We could also consider using the validation feature on the bitcoin crate to actually verify witnesses.

We shouldn't ever get to this point if that were the case, we should return an error when the inputs are provided.

Yip, exactly.

We could also consider using the validation feature on the bitcoin crate to actually verify witnesses.

Yeah we could and since we’re only verifying the ones provided by the client, we don’t add much to the “DoS budget” from an “untrusted” peer.

We could also consider using the validation feature on the bitcoin crate to actually verify witnesses.

I'm not super excited to start depending on this in non-test. Its been removed from Bitcoin Core so is slowly getting more and more outdated and I don't think anyone else uses it aside from us anyway. If we checked the list of scripts is in one of several standard formats, we can (and should, IMO) just use that knowledge to check things.

For P2TR script path spend we’d be uncertain if it was a signature or just random 64 bytes (65 with explicit sighash).

The solution to also look for corresponding op_checksigs becomes tricky since we’d also need to consider that for op_checksigadd where there is a threshold.

It could be done but you’d at least need to do some very basic script validation to see if the supposed signatures are consumed by checksig opcodes and then only verify the sighash type of those. Conditional branching in script further complicates that (even though people should probably use a different leaf script in that case).

It feels like we’d run into the problem of writing our own validation just to check this.

I can’t imagine we’d get many false positives if we assumed 64/65 byte things are signatures. It just feels wrong.

Discussed this in the meeting. Will push another update tomorrow re-adding scriptpubkeys to NegotiatedTxInput and fully verifying key-spends, but not P2WSH or P2TR script-path (we'll make it very clear the client is on their own as they should know what they're doing and we'll describe the consequences). We will check all signatures in witnesses use sighash_all as well.

lightning/src/ln/channel.rs

The `FundingTransactionReadyForSignatures` event requests witnesses from the client for their contributed inputs to an interactively constructed transaction. The client calls `ChannelManager::funding_transaction_signed` to provide the witnesses to LDK.

…igned exchange

…hecks In a following commit, We'll use the contained scriptPubKeys to validate P2WPKH and P2TR key path spends and to assist in checking that signatures in provided holder witnesses use SIGHASH_ALL to prevent funds being frozen or held ransom.

LDK checks the following: * Each input spends an output that is one of P2WPKH, P2WSH, or P2TR. These were already checked by LDK when the inputs to be contributed were provided. * All signatures use the `SIGHASH_ALL` sighash type. * P2WPKH and P2TR key path spends are valid (verifies signatures) NOTE: * When checking P2WSH spends, LDK tries to decode 70-72 byte witness elements as ECDSA signatures with a sighash flag. If the internal DER-decoding fails, then LDK just assumes it wasn't a signature and carries with checks. If the element can be decoded as an ECDSA signature, the the sighash flag must be `SIGHASH_ALL`. * When checking P2TR script-path spends, LDK assumes all elements of exactly 65 bytes with the last byte matching any valid sighash flag byte are schnorr signatures and checks that the sighash type is `SIGHASH_ALL`. If the last byte is not any valid sighash flag, the element is assumed not to be a signature and is ignored. Elements of 64 bytes are not checked because if they were schnorr signatures then they would implicitly be `SIGHASH_DEFAULT` which is an alias of `SIGHASH_ALL`.

We directly get the holder `TxSignatures` when necessary.

dunxen · 2025-07-17T09:59:25Z

lightning/src/ln/channel.rs

@@ -7605,6 +7586,188 @@ where
 		}
 	}

+	fn verify_interactive_tx_signatures(


Needs some clean up and tests but pushed it for some review.

wpaulino · 2025-07-17T22:37:04Z

lightning/src/events/mod.rs

-				// drop any V2-established/spliced channels which have not yet exchanged the initial `commitment_signed`.
-				// We only exhange the initial `commitment_signed` after the client calls
-				// `ChannelManager::funding_transaction_signed` and ALWAYS before we send a `tx_signatures`
+				// We never write out FundingTransactionReadyForSigning events as they will be regenerated necessary.


when necessary*

wpaulino · 2025-07-17T22:54:34Z

lightning/src/ln/channel.rs

@@ -1786,7 +1786,7 @@ where
 	#[rustfmt::skip]
 	pub fn commitment_signed<L: Deref>(
 		&mut self, msg: &msgs::CommitmentSigned, best_block: BestBlock, signer_provider: &SP, logger: &L
-	) -> Result<(Option<ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>, Option<ChannelMonitorUpdate>), ChannelError>
+	) -> Result<(Option<ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>, Option<ChannelMonitorUpdate>, Option<Transaction>), ChannelError>


Maybe it's time we make this an enum?

wpaulino · 2025-07-17T22:55:35Z

lightning/src/ln/channel.rs

@@ -1840,15 +1840,15 @@ where
 				let res = if has_negotiated_pending_splice && !session_received_commitment_signed {
 					funded_channel
 						.splice_initial_commitment_signed(msg, logger)
-						.map(|monitor_update_opt| (None, monitor_update_opt))
+						.map(|monitor_update_opt| (None, monitor_update_opt, None))


We will also need to return the unsigned transaction here for splices.

wpaulino · 2025-07-17T23:09:21Z

lightning/src/ln/channelmanager.rs

+					if let Some(unsigned_transaction) = funding_tx_opt {
+						let mut pending_events = self.pending_events.lock().unwrap();
+						pending_events.push_back((
+							Event::FundingTransactionReadyForSigning {


I think we'll also need to generate this on reestablish when we have already received commitment_signed but not tx_signatures.

wpaulino · 2025-07-17T23:14:13Z

lightning/src/ln/interactivetxs.rs

@@ -1529,7 +1538,13 @@ impl InteractiveTxInput {

 	fn into_negotiated_input(self) -> NegotiatedTxInput {
 		let weight = self.input.estimate_input_weight();
-		NegotiatedTxInput { serial_id: self.serial_id, txin: self.input.into_tx_in(), weight }
+		let prev_output = self.input.prev_output().clone();


Not a huge deal but seems like we could easily avoid the clone here?

wpaulino · 2025-07-17T23:16:34Z

lightning/src/ln/channel.rs

+			let all_prevouts = sighash::Prevouts::All(&prev_outputs[..]);
+
+			let mut cache = SighashCache::new(&built_tx);
+			let secp = Secp256k1::verification_only();


We should just pass through the existing secp context in ChannelContext

wpaulino · 2025-07-17T23:21:12Z

lightning/src/ln/channel.rs

+				}
+
+				// P2TR key path spend witness includes signature and optional annex
+				if script_pubkey.is_p2tr() && witness.len() <= 2 {


We should probably check that the second element is an annex as it should start with 0x50, but I think transactions with one aren't relayed/mined by default with Bitcoin Core yet.

wpaulino · 2025-07-17T23:21:56Z

lightning/src/ln/channel.rs

+	fn verify_interactive_tx_signatures(
+		&mut self, witnesses: &Vec<Witness>,
+	) -> Result<(), APIError> {
+		if let Some(session) = &self.interactive_tx_signing_session {


Needs an error when None

wpaulino · 2025-07-17T23:23:12Z

lightning/src/ln/channel.rs

+					|| self
+						.pending_splice
+						.as_ref()
+						.and_then(|pending_splice| Some(pending_splice.funding.is_some()))
+						.unwrap_or(false) && self
+						.context
+						.channel_state
+						.is_monitor_update_in_progress();


Could be worth having a helper for this

dunxen added Dual-funding Splicing labels Jun 24, 2025

dunxen requested review from wpaulino, optout21 and jkczyz June 24, 2025 12:13

jkczyz reviewed Jun 24, 2025

View reviewed changes

dunxen mentioned this pull request Jun 25, 2025

Contribute funding inputs on accepting dual-funded channel #3735

Draft

dunxen force-pushed the 2025-06-readyforsigningevent branch from 165d59d to 8ca6d79 Compare June 25, 2025 10:59

optout21 reviewed Jun 26, 2025

View reviewed changes

dunxen force-pushed the 2025-06-readyforsigningevent branch from 890633d to a1de384 Compare June 27, 2025 07:20

jkczyz reviewed Jun 27, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2025-06-readyforsigningevent branch 2 times, most recently from 7df5779 to c8f981c Compare June 30, 2025 10:29

jkczyz reviewed Jun 30, 2025

View reviewed changes

dunxen force-pushed the 2025-06-readyforsigningevent branch 2 times, most recently from c15f426 to ff1489d Compare July 1, 2025 09:18

dunxen force-pushed the 2025-06-readyforsigningevent branch from ff1489d to 0a586e6 Compare July 4, 2025 07:05

wpaulino reviewed Jul 7, 2025

View reviewed changes

dunxen force-pushed the 2025-06-readyforsigningevent branch 2 times, most recently from a9e1a3a to 83e78d6 Compare July 14, 2025 09:34

dunxen added 2 commits July 16, 2025 11:59

f only emit FundingTransactionReadyForSigning on initial commitment_s…

60fb2f4

…igned exchange

dunxen force-pushed the 2025-06-readyforsigningevent branch from 83e78d6 to 1ab3286 Compare July 16, 2025 11:38

dunxen added 4 commits July 16, 2025 22:05

f add splicing check in funding_transaction_signed

a285743

Use bool for monitor_pending_tx_signatures over Option<TxSignatures>

b161cba

We directly get the holder `TxSignatures` when necessary.

dunxen force-pushed the 2025-06-readyforsigningevent branch from 1ab3286 to b161cba Compare July 17, 2025 09:57

dunxen commented Jul 17, 2025

View reviewed changes

wpaulino reviewed Jul 17, 2025

View reviewed changes

Introduce FundingTransactionReadyForSignatures event #3889

Are you sure you want to change the base?

Introduce FundingTransactionReadyForSignatures event #3889

Conversation

dunxen commented Jun 24, 2025

Uh oh!

ldk-reviews-bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Jun 26, 2025

Uh oh!

ldk-reviews-bot commented Jun 26, 2025

Uh oh!

optout21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 28, 2025

Uh oh!

ldk-reviews-bot commented Jun 30, 2025

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dunxen commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 2, 2025

Uh oh!

ldk-reviews-bot commented Jul 5, 2025

Uh oh!

ldk-reviews-bot commented Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunxen Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Introduce `FundingTransactionReadyForSignatures` event #3889

Introduce `FundingTransactionReadyForSignatures` event #3889

ldk-reviews-bot commented Jun 24, 2025 •

edited

Loading

codecov bot commented Jun 25, 2025 •

edited

Loading

dunxen commented Jul 1, 2025 •

edited

Loading

dunxen Jul 14, 2025 •

edited

Loading